feat: make it an error to call CollectStatusEvent.add_status with error or unknown #1386
Conversation
There was a problem hiding this comment.
I think the overloads are fine - yes, there are 18 lines, but it's readable and clear what it's doing.
One concern I have is if people are using loosely typed strings to call from_name, or loosely typed collections of statuses when calling add_status, they'll now have a lot of type checking errors.
It would be good to check for this (e.g. super-tox -e with the various names charms use for static checking). I'm generally ok with going to the stricter types both for the ability to have more static checking and for any autocomplete improvements it brings (probably not much here, since these objects are practically identical), but I don't think we want to annoy charmers if there isn't a clear win.
There was a problem hiding this comment.
Sorry to be a wet blanket, but I'm not a fan of this. I think it's a lot of implementation noise for the sake of something that's likely not an issue in practice. I'd much prefer we keep the types simple and do the runtime checks.
However, I suggest we do that a different way. For one thing, this only covers CollectStatusEvent.add_status, not app.status = ... or unit.status = ..., and we'd want the same check for those.
If we put the check in status_set, we'd mirror the check that Juju's status-set hook tool already does. In fact, for the _TestingModelBackend used by harness, we already do this:
def status_set(self, status: '_StatusName', message: str = '', *, is_app: bool = False):
if status in [model.ErrorStatus.name, model.UnknownStatus.name]:
raise model.ModelError(
f'ERROR invalid status "{status}", expected one of'
' [maintenance blocked waiting active]'
)(It would seem better to do "not in allowed" rather than "in disallowed", but that's a minor detail.)
In other words, as long as someone has a unit test with the code path in question, Harness will already raise an error and flag this.
It looks like Scenario doesn't do this check (@tonyandrewmeyer can correct me if I'm wrong), but that could easily be added, and I'd be in support of that.
Overall it's a trade-off: we want reasonable types, but they come at a cost, and I think we need to be careful here.
Yeah I think it's a lot of typing ceremony for not much value added. I've taken out the type checking changes in favour of just runtime checks.
|
It doesn't, and I'd be happy have that added too (it would be in mocking.py's |
Ah, that's a good point -- thanks. |
|
I've also opened an issue in ops-scenario to make its |
|
Are we happy to merge the changes in as is? The current changes (1 approval) are an update to the docstring for |
This reverts commit e3edf3f.
…or or unknown (canonical#1386) Add a runtime check to immediately raise an InvalidStatusError if add_status is called with ErrorStatus or UnknownStatus. Previously these statuses would (in the case of ErrorStatus) or might (in the case of UnknownStatus) lead to an InvalidStatusError when it came time to set the status after the collect-status observer returned. It was never valid to set either status, this just means that the user will get an immediate exception that points to the line adding the bad status.
It seems like it should immediately be an error to add an
ErrorStatusorUnknownStatuswithadd_status, since these can never be set as the status, and will cause an error at the end of the event if they are the highest priority status added (always the case withErrorStatusas it has the highest priority, while withUnknownStatusonly if it is the only status type added, as it has the lowest priority). See also issue #1356.This draft PR makes this relatively simple runtime change, and then investigates the changes needed to support this via type checking.
There ends up being more type checking code than runtime code, with two new public type aliases (
SettableStatusandSettableStatusName-- better names might be better?).The biggest offender is
from_name, which previously took astrargument and (implicitly) returned anUnknownStatusor anotherStatusBase, but which we now need to know whether the status returned will be valid for use withadd_statusor not. This PR uses@overloadto accomplish this, but possibly there's a better way (aTypedDictforStatusBase._statusesperhaps?).One concern I have is if people are using loosely typed strings to call
from_name, or loosely typed collections of statuses when callingadd_status, they'll now have a lot of type checking errors.One possible solution would be to just make the runtime changes and leave the typing alone for now.